Skip to content

fix(sqlserver): escape SQL identifiers and literals in metadata/DDL paths (#1914) - #2196

Merged
openai0229 merged 4 commits into
OtterMind:mainfrom
HandSonic:fix/sqli2-sqlserver
Jul 29, 2026
Merged

fix(sqlserver): escape SQL identifiers and literals in metadata/DDL paths (#1914)#2196
openai0229 merged 4 commits into
OtterMind:mainfrom
HandSonic:fix/sqli2-sqlserver

Conversation

@HandSonic

@HandSonic HandSonic commented Jul 26, 2026

Copy link
Copy Markdown
Contributor

Related issue

Closes #1914

Summary

Completes the SQL Server database-plugin portion of the identifier and literal escaping work.

  • Consolidates bracket identifier and string-literal handling in SqlServerIdentifierProcessor and removes the parallel identifier utility.
  • Preserves conditional quoteIdentifier(...) semantics while using quoteIdentifierAlways(...) for generated DDL and inherited builder/manager paths.
  • Round-trips embedded and boundary brackets, handles qualified bracket/double-quoted identifiers, and uses locale-independent reserved-word matching.
  • Adds quote-aware validation for parameterized/user-defined types and legal SQL Server defaults, while rejecting comments, statement boundaries, unbalanced expressions, and column-clause injection.
  • Quotes database/schema/table/column/index names across metadata, export, update, template, drop, truncate, create, rename, and comment paths.
  • Uses one SQL Server literal escaper for metadata comments and text/XML/geography value serialization without incorrectly doubling backslashes.

Affected surfaces

  • Frontend / Web
  • Backend / API / Storage
  • Database plugin / Driver
  • JCEF / Desktop packaging
  • CI / Build / Release
  • Documentation only

Verification

  • Commands and results:
    • mvn -B -f chat2db-community-server/pom.xml -pl chat2db-community-plugins/chat2db-community-sqlserver -am clean test -DskipTests=false -Dmaven.test.skip=false -Dsurefire.failIfNoSpecifiedTests=false
    • Result: BUILD SUCCESS across 8 reactor modules; 188 tests passed in tested modules, including SQL Server 72/72, SPI 90/90, and tools 26/26.
    • git diff --check origin/main...HEAD: passed.
  • Manual verification: Source-level generated-SQL review completed. No live SQL Server instance was used.
  • UI evidence: N/A

Risk and compatibility

  • Public API or stored data: No public API or stored-data schema change. Uses the existing shared ISQLIdentifierProcessor contract.
  • Database or driver compatibility: Ordinary SQL Server identifiers retain conditional behavior; metadata-driven DDL now preserves bracket characters and qualified names. Legal defaults and custom/parameterized types are covered by regression tests.
  • Network, privacy, or security: No network or privacy change. Reduces malformed SQL and identifier/literal injection risk in generated metadata, DDL, export, and DML paths.
  • Community / Local / Pro boundary: Community SQL Server plugin only; Local and Pro consume the Community plugin behavior without product-specific code changes.
  • Backward compatibility: Existing ordinary identifiers, defaults, types, and values retain their emitted form; embedded delimiters and apostrophes are now encoded consistently.

Reviewer map

  • Start here: SqlServerIdentifierProcessor, SqlServerSqlGuards, then SqlServerSqlBuilder, SqlServerDBManager, and SqlServerColumnTypeEnum.
  • Failure condition: Bracket quote/remove does not round-trip, legal defaults/types are rewritten, generated SQL contains raw metadata identifiers/literals, or backslashes are doubled in SQL Server strings.
  • Rollback or disable path: Revert this PR; no migration or persisted-data rollback is required.

Contributor declaration

  • I linked the Issue that defines this change.
  • I tested the affected behavior and reported the actual results above.
  • I did not include credentials, private data, or generated build output.
  • I disclosed substantial AI assistance below, or this PR contains no substantial AI-generated code.

AI assistance: Substantial AI-assisted maintainer revision, review, and test expansion; all final changes were source-reviewed and verified with the commands above.

HandSonic and others added 4 commits July 29, 2026 16:58
…per maintainer review (OtterMind#1914)

- strengthen SqlServerIdentifierProcessor (SPI ISQLIdentifierProcessor):
  quoteIdentifier always bracket-quotes with ] doubling, escapeString
  doubles single quotes only (no backslash doubling in T-SQL)
- SqlServerMetaData.getSQLIdentifierProcessor() returns the shared
  INSTANCE; metadata call sites use it (statics use INSTANCE directly)
- builders/DBManager/enums/XML+Geography value processors rewired to
  SqlServerIdentifierProcessor.INSTANCE / static escapeIdentifier
- collation validation moved to public SqlServerSqlGuards
- SqlServerIdentifierUtils removed; tests migrated to
  SqlServerIdentifierProcessorTest (49 green)
@openai0229
openai0229 force-pushed the fix/sqli2-sqlserver branch from 48344f5 to 45719f6 Compare July 29, 2026 09:00

@openai0229 openai0229 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer review complete. Rebased onto current main, fixed SQL Server bracket/literal/default-expression semantics, and verified with the eight-module clean reactor suite (188/188 tests passed).

@openai0229
openai0229 merged commit a28ca03 into OtterMind:main Jul 29, 2026
19 checks passed
@openai0229 openai0229 moved this from In Review to Done in Chat2DB Community Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Security: 34 SQL Injection Vulnerabilities in Database Plugins

2 participants